group: refactor MPIR_Group #7235

hzhou · 2024-12-10T23:31:09Z

Pull Request Description

Hide the internal fields of MPIR_Group from unnecessary access.

Outside group_util.c and group_impl.c, it only need assume the MPIR_Lpid
integer type, creation routines based on lpid map or lpid stride
description, and access routine to look up lpid from a group rank.

Add feature to use stride to describe group composition

Remove the linked list design in MPIR_Group_pmap_t

[skip warnings]

Plan

Refactor MPIR_Group so it can be memory-efficient (strided rank map) -- this PR
Always create a communicator from MPIR_Group rather than the other way around
- This requires lpid to be device independent, and device layer perform address exchange upon communicator creation.
- Initially, we can require the first non-trivial communicator always to be MPI_COMM_WORLD.
Extend MPIR_Group to represent MPIR_Pset

Author Checklist

Provide Description
Particularly focus on why, not what. Reference background, issues, test failures, xfail entries, etc.
Commits Follow Good Practice
Commits are self-contained and do not do two things at once.
Commit message is of the form: module: short description
Commit message explains what's in the commit.
Passes All Tests
Whitespace checker. Warnings test. Additional tests via comments.
Contribution Agreement
For non-Argonne authors, check contribution agreement.
If necessary, request an explicit comment from your companies PR approval manager.

hzhou · 2024-12-11T14:29:28Z

test:mpich/ch3/most
test:mpich/ch4/most
All ✔️ other than 2 node crashes

hzhou · 2024-12-12T04:17:20Z

test:mpich/ch3/most

yfguo

Looking good overall. Need some attend w.r.t the reusing of map across groups.

src/mpi/group/grouputil.c

yfguo · 2025-01-15T00:12:49Z

src/mpi/group/grouputil.c

-    newgrp->rank = rank;
-    MPIR_Group_set_session_ptr(newgrp, session_ptr);
+        newgrp->pmap.use_map = true;
+        newgrp->pmap.u.map = map;


Is this shallow copy of map safe? Can MPICH code free a map before all groups using it are freed? We probably should refcount the map to safely reuse it across multiple MPIR_Group.

The map is owned by the group. The map is never shared. On the other hand, the MPIR_Group can be shared and it is reference counted.

src/mpi/group/grouputil.c

hzhou · 2025-02-07T22:44:55Z

test:mpich/ch3/most
test:mpich/ch4/most ✔️

hzhou · 2025-02-10T18:34:59Z

The last commit is likely missing some dependencies from the next PRs. Please review without the last commit. I'll drop the commit if I cannot resolve the issue in time.

raffenet

LGTM up until last commit

hzhou · 2025-02-11T03:33:21Z

test:mpich/ch3/most
test:mpich/ch4/most

hzhou · 2025-02-11T05:06:30Z

Fixed the last commit. The culprit was MPIR_Lpid typed as uint64_t. Changing it int64_t made the math (the stride pmap) work.

This test requires to access MPICH internals, thus won't be used with the current design.

We no longer use this file.

Hide the internal fields of MPIR_Group from unnecessary access. Outside group_util.c and group_impl.c, it only need assume the MPIR_Lpid integer type, creation routines based on lpid map or lpid stride description, and access routine to look up lpid from a group rank.

For most external usages, we only need MPIR_Group_rank_to_lpid.

Avoid access group internal fields.

Group similar functions together to facilitate refactoring. There is no changes in this commit other than moving functions around. The 4 incl/excl functions are very similar. The 3 difference/intersection/union functions are very similar.

Use MPIR_Group_{rank_to_lpid,lpid_to_rank} to avoid directly access MPIR_Group internal fields. For most group creation routines, just populate an lpid lookup map and call MPIR_Group_create_map to create the group.

* add option to use stride to describe group composition * remove the linked list design

The unsigned type uint64_t is dangerous as we perform pmap stride math. Signed integer should always work and we can use assertions to check its range.

* Add check_map_is_strided to detect strided pattern and convert a map into a strided pmap. * Move internal static routines to the bottom of grouputil.c.

yfguo

LGTM

dalcinl · 2025-02-12T05:54:34Z

@hzhou I believe this PR broke mpi4py tests:

testDifference (test_group.TestGroupSelf.testDifference) ... /home/runner/work/_temp/e71ecd9b-95ff-4ee1-97e1-da415480ba87.sh: line 1: 162928 Floating point exception(core dumped) python test/main.py -v

Full logs here. I only see useful logs with the runs using gforker, but not hydra. Maybe something else is going on.

hzhou · 2025-02-12T06:00:53Z

Not sure how it resulted in floating point exception consider we don't use floating point anywhere (other than reduction ops). I see it dumps core. Could you obtain a backtrace of it?

dalcinl · 2025-02-12T06:05:23Z

The UCX build is slightly more informative:

testDifference (test_group.TestGroupSelf.testDifference) ... [fv-az787-562:164480:0:164480] Caught signal 8 (Floating point exception: integer divide by zero)
==== backtrace (tid: 164480) ====
 0  /lib/libucs.so.0(ucs_handle_error+0x2e4) [0x7f47fb7c1a54]
 1  /lib/libucs.so.0(+0x34c4c) [0x7f47fb7c1c4c]
 2  /lib/libucs.so.0(+0x34fda) [0x7f47fb7c1fda]
 3  /lib/x86_64-linux-gnu/libc.so.6(+0x45320) [0x7f47fe645320]
 4  /usr/local/lib/libmpi.so.0(+0x57c720) [0x7f47fbd7c720]
 5  /usr/local/lib/libmpi.so.0(+0x57b86e) [0x7f47fbd7b86e]
 6  /usr/local/lib/libmpi.so.0(+0x578eaa) [0x7f47fbd78eaa]
 7  /usr/local/lib/libmpi.so.0(+0x16cd98) [0x7f47fb96cd98]
 8  /usr/local/lib/libmpi.so.0(PMPI_Group_difference+0x29) [0x7f47fb96ceed]

Full logs here.

dalcinl · 2025-02-12T08:08:31Z

mpi4py tests use group operations involving MPI_GROUP_EMPTY, and I suspect that's the place the 0 is coming from.
The backtrace points to file and line src/mpi/group/grouputil.c:452.

...
    } else {
        lpid -= pmap->u.stride.offset;
        MPIR_Lpid i_blk = lpid / pmap->u.stride.stride;
        MPIR_Lpid r_blk = lpid % pmap->u.stride.stride;
...

hzhou force-pushed the 2412_group branch 4 times, most recently from 576d5c7 to 0794b2f Compare December 11, 2024 14:23

hzhou requested a review from yfguo December 11, 2024 19:56

This was referenced Dec 13, 2024

comm: create local_group/remote_group before comm commit #7237

Merged

comm: re-implement dynamic processes using mpir-layer lpid #7240

Open

ch3: use group to build communicator vc tables #7242

Merged

yfguo requested changes Jan 15, 2025

View reviewed changes

hzhou force-pushed the 2412_group branch 2 times, most recently from 1535d80 to 351cd7f Compare February 7, 2025 22:44

hzhou force-pushed the 2412_group branch from 351cd7f to b5946f4 Compare February 10, 2025 17:06

hzhou requested review from yfguo and raffenet February 10, 2025 17:07

raffenet approved these changes Feb 10, 2025

View reviewed changes

hzhou force-pushed the 2412_group branch 3 times, most recently from ac5b69e to a78823b Compare February 11, 2025 03:32

hzhou requested a review from raffenet February 11, 2025 05:06

raffenet approved these changes Feb 11, 2025

View reviewed changes

hzhou added 4 commits February 11, 2025 12:02

test: remove unused test glpid

9a926b8

This test requires to access MPICH internals, thus won't be used with the current design.

group: remove unused groupdebug.c

bb2b550

We no longer use this file.

misc: use the new group rank/lpid conversion routines

6fc034b

For most external usages, we only need MPIR_Group_rank_to_lpid.

hzhou added 6 commits February 11, 2025 12:02

comm: use MPIR_Group_create_{map, stride)

08b7d05

Avoid access group internal fields.

group: rearange functions in group_impl.c

9042933

Group similar functions together to facilitate refactoring. There is no changes in this commit other than moving functions around. The 4 incl/excl functions are very similar. The 3 difference/intersection/union functions are very similar.

group: refactor group_impl.c to use new group interfaces

a834f5f

Use MPIR_Group_{rank_to_lpid,lpid_to_rank} to avoid directly access MPIR_Group internal fields. For most group creation routines, just populate an lpid lookup map and call MPIR_Group_create_map to create the group.

group: refactor MPIR_Group

fed0a7f

* add option to use stride to describe group composition * remove the linked list design

group: make MPIR_Lpid int64_t

37bf832

The unsigned type uint64_t is dangerous as we perform pmap stride math. Signed integer should always work and we can use assertions to check its range.

group: optimize grouputil

d5d81c7

* Add check_map_is_strided to detect strided pattern and convert a map into a strided pmap. * Move internal static routines to the bottom of grouputil.c.

hzhou force-pushed the 2412_group branch from a78823b to d5d81c7 Compare February 11, 2025 18:02

yfguo reviewed Feb 11, 2025

View reviewed changes

yfguo approved these changes Feb 11, 2025

View reviewed changes

hzhou merged commit 74357f0 into pmodels:main Feb 11, 2025
4 checks passed

hzhou deleted the 2412_group branch February 11, 2025 19:29

hzhou mentioned this pull request Feb 12, 2025

group: avoid divide by zero with empty group #7301

Merged

4 tasks

hzhou mentioned this pull request Mar 21, 2025

session: implement psets per session #7354

Draft

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

group: refactor MPIR_Group #7235

group: refactor MPIR_Group #7235

hzhou commented Dec 10, 2024 •

edited

Loading

hzhou commented Dec 11, 2024 •

edited

Loading

hzhou commented Dec 12, 2024

yfguo left a comment

yfguo Jan 15, 2025

hzhou Jan 15, 2025

hzhou commented Feb 7, 2025 •

edited

Loading

hzhou commented Feb 10, 2025

raffenet left a comment

hzhou commented Feb 11, 2025

hzhou commented Feb 11, 2025

yfguo left a comment

dalcinl commented Feb 12, 2025

hzhou commented Feb 12, 2025

dalcinl commented Feb 12, 2025

dalcinl commented Feb 12, 2025

group: refactor MPIR_Group #7235

group: refactor MPIR_Group #7235

Conversation

hzhou commented Dec 10, 2024 • edited Loading

Pull Request Description

Plan

Author Checklist

hzhou commented Dec 11, 2024 • edited Loading

hzhou commented Dec 12, 2024

yfguo left a comment

Choose a reason for hiding this comment

yfguo Jan 15, 2025

Choose a reason for hiding this comment

hzhou Jan 15, 2025

Choose a reason for hiding this comment

hzhou commented Feb 7, 2025 • edited Loading

hzhou commented Feb 10, 2025

raffenet left a comment

Choose a reason for hiding this comment

hzhou commented Feb 11, 2025

hzhou commented Feb 11, 2025

yfguo left a comment

Choose a reason for hiding this comment

dalcinl commented Feb 12, 2025

hzhou commented Feb 12, 2025

dalcinl commented Feb 12, 2025

dalcinl commented Feb 12, 2025

hzhou commented Dec 10, 2024 •

edited

Loading

hzhou commented Dec 11, 2024 •

edited

Loading

hzhou commented Feb 7, 2025 •

edited

Loading